manual song post creation - #435
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a validated songbook post generator, a CLI entry point, and a manually triggered GitHub Actions workflow. The workflow generates and formats posts, runs tests, creates a pull request, and enables automatic rebase merging. ChangesSongbook post generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant createSongbookPostScript
participant createSongbookPostFile
participant PullRequest
GitHubActions->>createSongbookPostScript: Pass post inputs
createSongbookPostScript->>createSongbookPostFile: Create validated post
GitHubActions->>GitHubActions: Format content and run tests
GitHubActions->>PullRequest: Create pull request and enable auto-merge
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| export function formatFrontmatterValue(value: unknown): string { | ||
| if (typeof value === "string") { | ||
| return `"${value.replace(/"/g, '\\"')}"`; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/create-songbook-post.yml:
- Around line 55-59: Remove the workflow_dispatch branch input and update the
job to always check out main. Replace DATA_PIPELINE_PR_AUTOMATION_KEY with the
scoped GITHUB_TOKEN for checkout, and set persist-credentials: false so
credentials are not retained; handle any non-main refs only in a separate job
without write permissions or secrets.
In `@src/lib/createSongbookPost.ts`:
- Around line 66-71: Update date handling in createSongbookPost so any non-empty
input.date must match the YYYY-MM-DD format and parse successfully; throw a
validation error when it does not instead of omitting it. Store and serialize
only the validated date value in validationObj and the frontmatter-writing logic
around lines 101-103, and add a regression test covering an invalid date.
- Around line 157-172: Validate cleanFilename in createSongbookPost before
constructing filePath, rejecting path separators and traversal segments so
filename cannot escape targetDir; alternatively resolve the candidate path and
verify it remains within targetDir before calling fs.writeFileSync. Preserve
normal .md/.mdx stripping and post creation for safe slugs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96c28cd0-ecb9-4beb-bac8-b1aef22349ae
📒 Files selected for processing (5)
.github/workflows/create-songbook-post.ymlbin/create-songbook-post.tspackage.jsonsrc/lib/createSongbookPost.test.tssrc/lib/createSongbookPost.ts
| branch: | ||
| description: "The branch to run on" | ||
| required: true | ||
| default: "main" | ||
| type: string |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eo pipefail
echo "== file exists =="
git ls-files | grep -FxF ".github/workflows/create-songbook-post.yml" || true
echo "== diff stat =="
git diff --stat # from HEAD if possible
echo "== workflow relevant sections =="
if [ -f .github/workflows/create-songbook-post.yml ]; then
nl -ba .github/workflows/create-songbook-post.yml | sed -n '1,130p'
fi
echo "== find create-pull-request and automation key usages =="
rg -n "create-pull-request|DATA_PIPELINE_PR_AUTOMATION_KEY|pull_request_token|persist-credentials|actions/checkout|branch:" .github/workflows . -g '!node_modules' -g '!dist' -g '!build' | head -200Repository: jtzero/cle-gunners
Length of output: 307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow relevant sections =="
awk '{printf "%6d %s\n", NR, $0}' .github/workflows/create-songbook-post.yml | sed -n '1,140p'
echo "== focused search =="
grep -RInE 'create-pull-request|DATA_PIPELINE_PR_AUTOMATION_KEY|pull_request_token|persist-credentials|actions/checkout|inputs\.branch|refs/heads|github.ref' .github/workflows . -g '!node_modules' -g '!dist' -g '!build' | head -200
echo "== package scripts =="
if [ -f package.json ]; then
python3 - <<'PY'
import json
with open("package.json") as f:
data=json.load(f)
print({k:data.get(k) for k in ("scripts","dependencies","devDependencies")})
PY
fiRepository: jtzero/cle-gunners
Length of output: 5423
🌐 Web query:
GitHub Actions workflow_dispatch user permissions write access required
💡 Result:
To manually trigger a GitHub Actions workflow using the workflow_dispatch event, write access to the repository is required [1][2][3]. Key details regarding permissions and requirements: 1. Repository Access: Users or service accounts (via tokens) must have write access to the repository to trigger a workflow manually via the GitHub UI, GitHub CLI, or REST API [1][4][3]. 2. Workflow Location: The workflow file must exist on the repository's default branch for the workflow_dispatch trigger to be active and recognized by GitHub [1][5][3]. 3. Authentication Scopes: When using API tokens (Personal Access Tokens or OAuth tokens), the required permissions typically involve repository write access (often represented by the repo scope for classic tokens) or specific fine-grained permissions such as actions:write [6][4][7]. 4. UI Visibility: If a user does not have write access, they will not see the Run workflow button in the GitHub Actions tab, even if the workflow is properly configured [1][2]. In summary, write access is a fundamental requirement for manually invoking workflows configured with workflow_dispatch, as it ensures only authorized users can initiate these actions [1][2][3].
Citations:
- 1: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/manually-run-a-workflow?tool=webui
- 2: Fix workflow_dispatch security statement github/docs#2179
- 3: https://github.com/github/docs/blob/main/content/actions/how-tos/manage-workflow-runs/manually-run-a-workflow.md
- 4: https://stackoverflow.com/questions/73400268/github-actions-must-have-admin-rights-to-trigger-workflow-dispatch
- 5: https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/trigger-a-workflow
- 6: https://docs.github.com/rest/actions/workflows
- 7: https://github.com/step-security/dispatch-workflow
🌐 Web query:
npm install runs lifecycle scripts install lifecycle documentation
💡 Result:
In npm, lifecycle scripts are automated hooks defined in the scripts property of a package.json file that execute at specific stages of package installation and management [1][2][3]. As of July 2026, with the release of npm v12, significant security changes have been implemented regarding these scripts [4]. Lifecycle Scripts and npm install When running npm install, the following lifecycle scripts are standard: - preinstall: Executes before the package is installed [3][4]. - install: Executes after the package is installed [3][4]. - postinstall: Executes after the package is installed [3][4]. Important Security Change (npm v12+) Prior to npm v12, these scripts executed automatically by default, posing significant supply chain security risks [5][6][4]. Starting with npm v12 (released July 2026), all dependency install scripts are disabled by default [4]. Furthermore, this protection now extends to the implicit node-gyp rebuild process (triggered by binding.gyp files), which previously acted as a "Phantom Gyp" bypass for security controls [4]. Packages that require these scripts must now be explicitly allowlisted in the project's npm configuration [7][4]. Execution Order During an npm install command, the lifecycle scripts for the project and its dependencies typically follow this general sequence [3]: 1. preinstall 2. install 3. postinstall 4. prepublish 5. preprepare 6. prepare 7. postprepare Note that recent updates have refined the execution of root-level preinstall scripts to ensure they run before dependency resolution (reify) begins, addressing long-standing issues where root scripts could not effectively gate the installation process [8]. Management and Environment Variables - Environment Variable: The npm_lifecycle_event environment variable is set during execution, allowing a single script to detect which lifecycle stage is currently active [1][9][2]. - Best Practices: Users should avoid relying on automatic script execution for critical build steps and should instead use explicit, controlled build processes [5][10]. Reviewing the hasInstallScript flag in package-lock.json is recommended for identifying dependencies that utilize these hooks [4].
Citations:
- 1: https://docs.npmjs.com/cli/v11/using-npm/scripts/
- 2: https://github.com/npm/cli/blob/e2346e7702acccefe6d711168c2b0e0e272e194a/doc/misc/npm-scripts.md
- 3: https://deepwiki.com/npm/cli/6.4-lifecycle-scripts
- 4: https://cve.optibot.re/blog/npm-install-scripts-security-2026
- 5: https://safeguard.sh/resources/blog/npm-lifecycle-scripts-security-risks
- 6: [RFC] Make install scripts opt-in npm/rfcs#868
- 7: https://github.com/npm/rfcs/blob/main/accepted/0054-make-scripts-install-opt-in.md
- 8: fix!: run root preinstall before reify npm/cli#9267
- 9: https://docs.npmjs.com/cli/v8/using-npm/scripts/?v=true
- 10: https://snyk.io/blog/inside-keyv-npm-compromise-preinstall-malware-trusted-provenance-ide-hooks/
Do not run dispatcher-selected refs with the automation key.
workflow_dispatch requires write access, and this job checks out $inputs.branch with DATA_PIPELINE_PR_AUTOMATION_KEY before npm install. Even if root install scripts are disabled, selected repository code can still execute with that credential and expose it. Remove the branch input, checkout main, use the scoped GITHUB_TOKEN, and set persist-credentials: false. If non-main refs are required, run them in a separate job without write permissions or secrets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/create-songbook-post.yml around lines 55 - 59, Remove the
workflow_dispatch branch input and update the job to always check out main.
Replace DATA_PIPELINE_PR_AUTOMATION_KEY with the scoped GITHUB_TOKEN for
checkout, and set persist-credentials: false so credentials are not retained;
handle any non-main refs only in a separate job without write permissions or
secrets.
Source: Linters/SAST tools
| if (input.date && input.date.trim() !== "") { | ||
| const parsedDate = new Date(input.date.trim()); | ||
| if (!isNaN(parsedDate.getTime())) { | ||
| validationObj.date = parsedDate; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject invalid dates before serializing frontmatter.
An invalid non-empty date is omitted from validationObj, so postSchema.safeParse can succeed. Lines 101-103 then write that same invalid value into the file. The generated post can fail content parsing after this function reports success.
Require a valid YYYY-MM-DD value and throw when parsing fails. Serialize only the validated value. Add a regression test for an invalid date.
Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/createSongbookPost.ts` around lines 66 - 71, Update date handling in
createSongbookPost so any non-empty input.date must match the YYYY-MM-DD format
and parse successfully; throw a validation error when it does not instead of
omitting it. Store and serialize only the validated date value in validationObj
and the frontmatter-writing logic around lines 101-103, and add a regression
test covering an invalid date.
| let cleanFilename = input.filename.trim(); | ||
| if (cleanFilename.endsWith(".md")) { | ||
| cleanFilename = cleanFilename.slice(0, -3); | ||
| } | ||
| if (cleanFilename.endsWith(".mdx")) { | ||
| cleanFilename = cleanFilename.slice(0, -4); | ||
| } | ||
|
|
||
| const markdown = generateSongbookPostMarkdown(input); | ||
| const targetDir = path.join(baseDir, "src", "content", "posts"); | ||
| if (!fs.existsSync(targetDir)) { | ||
| fs.mkdirSync(targetDir, { recursive: true }); | ||
| } | ||
|
|
||
| const filePath = path.join(targetDir, `${cleanFilename}.md`); | ||
| fs.writeFileSync(filePath, markdown, "utf-8"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict filename to a safe post slug.
cleanFilename accepts .. path segments. path.join normalizes those segments, so a caller can write outside src/content/posts. Reject path separators and traversal segments, or resolve the path and verify that it remains under targetDir.
Proposed fix
- let cleanFilename = input.filename.trim();
- if (cleanFilename.endsWith(".md")) {
- cleanFilename = cleanFilename.slice(0, -3);
- }
- if (cleanFilename.endsWith(".mdx")) {
- cleanFilename = cleanFilename.slice(0, -4);
+ const cleanFilename = input.filename
+ .trim()
+ .replace(/\.(?:md|mdx)$/i, "");
+ if (!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(cleanFilename)) {
+ throw new Error("filename must be a lowercase kebab-case post slug");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cleanFilename = input.filename.trim(); | |
| if (cleanFilename.endsWith(".md")) { | |
| cleanFilename = cleanFilename.slice(0, -3); | |
| } | |
| if (cleanFilename.endsWith(".mdx")) { | |
| cleanFilename = cleanFilename.slice(0, -4); | |
| } | |
| const markdown = generateSongbookPostMarkdown(input); | |
| const targetDir = path.join(baseDir, "src", "content", "posts"); | |
| if (!fs.existsSync(targetDir)) { | |
| fs.mkdirSync(targetDir, { recursive: true }); | |
| } | |
| const filePath = path.join(targetDir, `${cleanFilename}.md`); | |
| fs.writeFileSync(filePath, markdown, "utf-8"); | |
| const cleanFilename = input.filename | |
| .trim() | |
| .replace(/\.(?:md|mdx)$/i, ""); | |
| if (!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(cleanFilename)) { | |
| throw new Error("filename must be a lowercase kebab-case post slug"); | |
| } | |
| const markdown = generateSongbookPostMarkdown(input); | |
| const targetDir = path.join(baseDir, "src", "content", "posts"); | |
| if (!fs.existsSync(targetDir)) { | |
| fs.mkdirSync(targetDir, { recursive: true }); | |
| } | |
| const filePath = path.join(targetDir, `${cleanFilename}.md`); | |
| fs.writeFileSync(filePath, markdown, "utf-8"); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 171-171: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(filePath, markdown, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/createSongbookPost.ts` around lines 157 - 172, Validate cleanFilename
in createSongbookPost before constructing filePath, rejecting path separators
and traversal segments so filename cannot escape targetDir; alternatively
resolve the candidate path and verify it remains within targetDir before calling
fs.writeFileSync. Preserve normal .md/.mdx stripping and post creation for safe
slugs.
Source: Linters/SAST tools
bc44c67 to
c78a5f6
Compare
2 participants
Summary by CodeRabbit
New Features
Tests